Reviewer role: Principal Staff Engineer
Branch: RHOAIENG-71231-define-the-proper-mapping-of-previous-chat-completions-requests-parameters-to-responses-requests-ones
Base: main

1. High-Level Summary

Goal: Map HPO (Hyperparameter Optimization) Chat Completions prompt parameters to the exported Responses API input[system] format. The core challenge is that HPO prompt templates embed OGX-owned runtime-injection strings (citation instructions, retrieval grounding rules, document slots) that must be stripped before being exported as a static Responses API system prompt — otherwise the API payload would duplicate text that OGX injects at inference time via file_search.

Scope:

• ~420 lines of new production code in pattern_builder.py (a new build_responses_system_input() entry-point + ~15 private helpers)
• 2-line change in experiment.py to propagate temperature and max_completion_tokens into the generation dict
• ~320 lines of new tests in test_pattern_builder.py

2. Confirmed Issues & Critical Risks

[HIGH] — tool_choice schema is structurally invalid

File: pattern_builder.py:460

"tool_choice": {"mode": "required", "tools": [{}], "type": "file_search"},

"tools": [{}] is an array containing one empty object. This matches no documented shape of the OpenAI/RHOAI Responses API. The tool_choice object for forced file-search should likely be {"type": "file_search"} (or whatever the deployment-specific schema requires). An empty dict inside the tools array will either silently be ignored or cause a 400-validation error from the API at runtime — with no test catching it since tests only assert the dict value, not validate it against the API spec.

Fix: Verify the exact tool_choice contract with the Responses API team and replace the literal. At minimum add an integration-level test or schema-validation fixture so this doesn't silently drift again.

[HIGH] — _system_has_grounding_policy uses substring (in) while all other checkers use prefix match

File: pattern_builder.py:303

normalized = system.lower()
return any(prefix.lower() in normalized for prefix in _GROUNDING_PREFIXES)

Every other function that checks _GROUNDING_PREFIXES (e.g. _sentence_is_ogx_duplicative, _should_skip_redundant_user_line) uses startswith. This one uses in — a full-text substring scan. A system prompt like "Use only relevant information from documents below the line." or "All documents do not contain user PII" would incorrectly register as having a grounding policy, causing user-template grounding lines to be over-suppressed.

Fix:

def _system_has_grounding_policy(system: str) -> bool:
    sentences = [s.strip() for s in re.split(r"(?<=[.!?])\s+", system)]
    return any(
        any(sent.lower().startswith(p.lower()) for p in _GROUNDING_PREFIXES)
        for sent in sentences
    )

[HIGH] — _strip_ogx_runtime_instructions double-replaces and risks over-stripping

File: pattern_builder.py:170-172

for phrase in _SYSTEM_GROUNDING_PHRASES:
    text = text.replace(phrase, "").replace(phrase.rstrip("."), "")

For phrase = "Answer using ONLY the provided documents.", after the first .replace(phrase, "") removes it (if present), the second .replace(phrase.rstrip("."), "") then scans for the substring "Answer using ONLY the provided documents" in the already-modified string. If another sentence like "Answer using ONLY the provided documents reliably and succinctly." exists, the rstrip version could partially match it, silently excising content mid-sentence.

Fix: Apply both replacements independently on the original text value before compounding:

for phrase in _SYSTEM_GROUNDING_PHRASES:
    text = text.replace(phrase, "")
    text = text.replace(phrase.rstrip("."), "")

Or, preferably, use a single regex with word-boundary anchoring to avoid mid-sentence partial matches.

[HIGH] — temperature and max_completion_tokens may be None; no guard in the builder

File: experiment.py:335-336, pattern_builder.py:458-459

# experiment.py
"temperature": foundation_model.params.temperature,
"max_completion_tokens": foundation_model.params.max_completion_tokens,

# pattern_builder.py
"max_output_tokens": generation["max_completion_tokens"],
"temperature": generation["temperature"],

If params.temperature or params.max_completion_tokens is None (e.g. for models with optional sampling), the Responses API will receive "temperature": null. The API contract for null-vs-omitted is often not the same. No test exercises the None case. The test_build_pattern_json_requires_generation_model_id test covers a missing model_id but not missing temperature/max_completion_tokens.

Fix: Guard in build_pattern_json:

if generation.get("temperature") is not None:
    rt["temperature"] = generation["temperature"]
if generation.get("max_completion_tokens") is not None:
    rt["max_output_tokens"] = generation["max_completion_tokens"]

Also add a test:

def test_omits_temperature_when_none():
    pattern = _make_pattern()
    pattern["settings"]["generation"]["temperature"] = None
    build_pattern_json(pattern)
    assert "temperature" not in pattern["settings"]["responses_template"]

[MEDIUM] — _normalize_answer_scaffold produces malformed output for non-canonical scaffold order

File: pattern_builder.py:159-162

def _normalize_answer_scaffold(line: str) -> str:
    normalized = line.replace(", with citations", "").replace("with citations", "")
    return _collapse_whitespace(normalized)

For "Answer (with citations, max 150 words):", the first replace removes nothing (the substring is "with citations," not ", with citations"), so the second replace removes "with citations", yielding "Answer (, max 150 words):" — a leading orphaned comma. The test only covers the canonical form "Answer (max 150 words, with citations):".

Fix: Use a regex:

def _normalize_answer_scaffold(line: str) -> str:
    normalized = re.sub(r",?\s*with citations,?\s*", "", line)
    return _collapse_whitespace(normalized)

[MEDIUM] — ranker_alpha != 1 mixes integer and float comparison semantics

File: pattern_builder.py:482

elif search_mode == "hybrid" and ranker_strategy == "weighted" and ranker_alpha is not None and ranker_alpha != 1:

The comment immediately below says "ranker_alpha == 1.0 intentionally falls through" but the guard checks != 1 (integer literal). Python's 1.0 == 1 is True, so behaviour is correct, but a future reader (or linter rule) may flag this as a type inconsistency, and someone maintaining the logic may change it to != 1.0 believing they're doing a cleanup when they're not.

Fix: Use a float literal and a tolerance for floating-point safety:

import math
elif ... and ranker_alpha is not None and not math.isclose(ranker_alpha, 1.0):

[MEDIUM] — _USER_RAG_GROUNDING_PREFIXES and _USER_GROUNDING_SKIP_PREFIXES overlap with _GROUNDING_PREFIXES without documented rationale

File: pattern_builder.py:85-93

There are three separate prefix collections for grounding-like suppression with non-obvious differences in where each is applied:

• _GROUNDING_PREFIXES: used in sentence-level and system detection
• _USER_GROUNDING_SKIP_PREFIXES: a subset, used only in pass-2 user-line filtering
• _USER_RAG_GROUNDING_PREFIXES: only suppressed in pass-1 when system_has_grounding is True

A new contributor has no way to know when to add a new OGX phrase to which list. The module-level comment says "update these lists" but doesn't explain the tripartite structure.

Fix: Add inline comments to each list explaining the specific pipeline stage where it applies, and why the membership criteria differ.

3. Maintainability & Readability

[MEDIUM] — Two-pass filtering bleeds concerns across pass boundaries

The docstring for build_responses_system_input describes a "Pass 1 / Pass 2" two-pass architecture. In practice, pass-1 (_filter_static_user_for_responses) also strips citation lines via _is_citation_related_line, which is a pass-2 concern. And pass-2 (_adapt_static_user_for_responses_export) also checks _USER_GROUNDING_SKIP_PREFIXES, which overlaps with pass-1 deduplication. The abstraction is leaky — what the caller sees as two distinct semantics is, at the line level, the same logic duplicated with slightly different predicate sets.

Recommendation: Either commit fully to the two-pass model with clear, non-overlapping predicate sets, or collapse to a single-pass predicate union and document the single comprehensive filter.

[LOW] — _is_citation_related_line checks _CITATION_PREFIXES then _HPO_CITATION_FRAGMENTS redundantly

File: pattern_builder.py:123-133

_CITATION_PREFIXES contains "You MUST cite sources". _HPO_CITATION_FRAGMENTS contains strings that all start with "You MUST cite sources ...". The startswith(_CITATION_PREFIXES) check on line 129 will always catch any _HPO_CITATION_FRAGMENTS hit before the second check on line 131 fires. The second check is unreachable for those fragments.

[LOW] — Module-level constants encode brittle external knowledge without a link to the source of truth

The extensive constant lists (_CITATION_PREFIXES, _FILE_SEARCH_MARKERS, etc.) are described as needing to stay in sync with benchmarking/rag/config.yaml. There is no automated check that these are in sync. Any OGX config change that adds a new injection phrase silently bypasses all filtering without any test failure.

Recommendation: Add a CI step or at least a comment pointing to the specific YAML keys that govern each list, so reviewers know what to check when config.yaml changes.

[LOW] — Test case numbering is out of order

File: test_pattern_builder.py, test_build_responses_system_input_handles_empty_inputs

The inline comments label the cases "Case 1", "Case 2", "Case 4", "Case 3" — Case 4 appears before Case 3 in source order. Minor, but breaks readability.

[LOW] — build_pattern_json mutates its input and returns it — the return value adds no information

The function signature implies a transformation returning a new value. In practice pattern is mutated in-place and then returned. The return value is the same object. Either the function should be renamed/documented to make the mutation explicit, or it should create and return a deep copy.

4. Test Coverage Gaps

5. Actionable Fixes

Fix 1 — tool_choice schema (HIGH)

Rationale: Protects against a silent API contract violation.

# Current (line 460) — suspicious empty object in tools array:
"tool_choice": {"mode": "required", "tools": [{}], "type": "file_search"},

# Proposed — verify the actual API spec, then use explicit shape:
"tool_choice": {"type": "file_search"},  # or as per Responses API documentation

Fix 2 — _system_has_grounding_policy over-matching (HIGH)

Rationale: Prevents spurious suppression of user supplements when a system prompt mentions documents in a non-grounding context.

def _system_has_grounding_policy(system: str) -> bool:
    sentences = [s.strip() for s in re.split(r"(?<=[.!?])\s+", system)]
    return any(
        any(sent.lower().startswith(p.lower()) for p in _GROUNDING_PREFIXES)
        for sent in sentences
    )

Fix 3 — _strip_ogx_runtime_instructions double-replace risk (HIGH)

Rationale: Eliminates mid-sentence partial-match over-stripping.

for phrase in _SYSTEM_GROUNDING_PHRASES:
    text = text.replace(phrase, "")
    text = text.replace(phrase.rstrip("."), "")
text = _collapse_whitespace(text)

Fix 4 — Guard None temperature/tokens (HIGH)

Rationale: Prevents sending null to the Responses API for optional fields.

# In build_pattern_json, after building the base template dict:
if generation.get("temperature") is not None:
    pattern["settings"]["responses_template"]["temperature"] = generation["temperature"]
if generation.get("max_completion_tokens") is not None:
    pattern["settings"]["responses_template"]["max_output_tokens"] = generation["max_completion_tokens"]

Add corresponding test:

def test_omits_temperature_when_none():
    pattern = _make_pattern()
    pattern["settings"]["generation"]["temperature"] = None
    build_pattern_json(pattern)
    assert "temperature" not in pattern["settings"]["responses_template"]

Fix 5 — _normalize_answer_scaffold regex (MEDIUM)

Rationale: Handles non-canonical comma order without producing orphaned punctuation.

def _normalize_answer_scaffold(line: str) -> str:
    normalized = re.sub(r",?\s*with citations,?\s*", "", line)
    return _collapse_whitespace(normalized)

Fix 6 — ranker_alpha floating-point comparison (MEDIUM)

Rationale: Makes floating-point intent explicit and avoids future type-confusion bugs.

import math
elif (
    search_mode == "hybrid"
    and ranker_strategy == "weighted"
    and ranker_alpha is not None
    and not math.isclose(ranker_alpha, 1.0)
):

Fix 7 — Test case label order (LOW)

File: test_pattern_builder.py

Renumber the inline comments to # Case 1, # Case 2, # Case 3, # Case 4 in source order inside test_build_responses_system_input_handles_empty_inputs.

Fix 8 — while "\n\n\n" loop (LOW)

Rationale: Consistent with the existing re.sub in _collapse_whitespace; handles any run of 3+ newlines in one pass.

# Replace (pattern_builder.py:194-195):
while "\n\n\n" in result:
    result = result.replace("\n\n\n", "\n\n")

# With:
result = re.sub(r"\n{3,}", "\n\n", result)